Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use otel4s-semconv-metrics-experimental for semantic testing #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Sep 12, 2024

Here is an example of the 'semantic conventions tests'.

@iRevive iRevive requested a review from NthPortal September 12, 2024 17:34
@@ -179,7 +179,7 @@ object OtelMetrics {
Meter[F]
.upDownCounter[Long](s"http.$kind.active_requests")
.withUnit("{request}")
.withDescription("Number of active HTTP requests.")
.withDescription(s"Number of active HTTP $kind requests.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've spotted the first mismatch 🥳

@iRevive
Copy link
Contributor Author

iRevive commented Sep 12, 2024

The current implementation of the MetricOps is rather limiting and we cannot comply the spec.

I don't think we should make drastic changes to 0.23, but we can to 1.0.x: http4s/http4s#7491.

@NthPortal
Copy link
Contributor

it seems I'll need to poke at updating the implementations of the tracing middlewares to be correct

@iRevive iRevive force-pushed the feature/semantic-testing branch from f52eb1d to 6497001 Compare September 24, 2024 17:06
@NthPortal NthPortal force-pushed the feature/semantic-testing branch from 6497001 to 904b670 Compare October 8, 2024 15:30
@NthPortal
Copy link
Contributor

@iRevive I rebased, fixed the conflicts, and force pushed. how do you want to proceed given that the current implementation from 0.23.x doesn't support some attributes? should we add logic to specifically skip the checks for those attributes and add a TODO comment to fix it in a later release? should we wait until we actually have a compliant implementation to fix it? something else?

I don't think we should make drastic changes to 0.23, but we can to 1.0.x

it should be possible to do it in a binary compatible way in 0.23.x, though it would be a pain. but would it be worth it anyway, as it may yet be many years until 1.0.x is released?

@iRevive
Copy link
Contributor Author

iRevive commented Oct 8, 2024

should we wait until we actually have a compliant implementation to fix it?

I think it would be the easiest option for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants